cmd/rofl/machine: Support provider:machine-id directly #680
Conversation
✅ Deploy Preview for oasisprotocol-cli ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
matevz
force-pushed
the
matevz/feature/rofl-machine-id
branch
2 times, most recently
from
3c5a323 to
61b5349
Compare
matevz
force-pushed
the
matevz/feature/rofl-machine-id
branch
2 times, most recently
from
59fa883 to
8d48aed
Compare
peternose
left a comment
peternose
left a comment
There was a problem hiding this comment.
Looks like these changes should work, but I did not test them. The code (previous and changes) is quite complicated and could be improved.
cmd/rofl/machine/mgmt.go
Outdated
| ) | ||
|
|
||
| func resolveMachine(args []string, deployment *buildRofl.Deployment) (*buildRofl.Machine, string, roflmarket.InstanceID) { | ||
| func resolveMachineManifestNpa(args []string, manifestOpts *roflCommon.ManifestOptions) (machineCfg *buildRofl.Machine, machineName string, machineID roflmarket.InstanceID, manifest *buildRofl.Manifest, extraCfg *roflCmdBuild.AppExtraConfig, providerAddr *types.Address, npa *common.NPASelection) { |
There was a problem hiding this comment.
Few remarks:
- Returned data should be collected in a struct, as Go methods should return at most 2 values.
- The returned values should also not be named, unless this is needed because of
deferstatement or to explain what the function returns. - The code is very complicated, and should be changed in such a way that you a) get npa b) resolve machine from args or manifest c) resolve provider address.
| Aliases: []string{"cancel", "rm"}, | ||
| Args: cobra.MaximumNArgs(1), | ||
| Run: func(_ *cobra.Command, args []string) { | ||
| txCfg := common.GetTransactionConfig() |
There was a problem hiding this comment.
Same could be done in change-admin.
cmd/rofl/machine/mgmt.go
Outdated
| return machineCfg, machineName, machineID, manifest, extraCfg, providerAddr, npa | ||
| } | ||
|
|
||
| func resolveMachineFromArgs(args []string) (*buildRofl.Machine, string, roflmarket.InstanceID) { |
There was a problem hiding this comment.
If function optionally returns values, it should return two values, the second one being bool, e.g. func resolve() (*mystruct, bool).
Also, happy path should not be indented.
if len(args) == 0
...
if len(parts) != 2
...
cmd/rofl/machine/mgmt.go
Outdated
|
|
||
| if err = manifest.Save(); err != nil { | ||
| cobra.CheckErr(fmt.Errorf("failed to update manifest: %w", err)) | ||
| if manifest != nil { |
There was a problem hiding this comment.
This complicates everything. Since helper function can return nil manifest, I would expect it could also return nil for other values. So I would expect these kind of checks for all values. Therefore, I don't allow returning nil values, and if I do, the function returns also bool to indicate whether the value is nil.
matevz
force-pushed
the
matevz/feature/rofl-machine-id
branch
from
8d48aed to
28139d5
Compare
This PR adds support for addressing ROFL machines directly using
provider-address:machine-idsyntax. This enables showing machine info, removing it, topping it up, changing admin, showing logs without needingrofl.yamlmanifest.Useful for automation. Related #677 .